Repository navigation
Conversation
…y and pbkdf2 In both Bun.password.verify/verifySync and PBKDF2::from_js, the preceding get_zig_string()/to_slice() call already propagates any JS exception via the ? operator. The code that follows (algorithm_from_zig_string / evp::lookup_ignore_case / Algorithm::md) is pure byte comparison and static-table lookup with no JS entry, so has_exception() is always false at the guard and the fallthrough branches (Err(JsError::Thrown) / Ok(JSValue::ZERO)) are dead. Hoist the pbkdf2 digest slice out of the 'invalid block so the error message reuses it instead of calling to_slice() a second time. No behavior change.
|
Status: dead-code cleanup in |
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — dead-code removal with no behavior change.
What was reviewed:
- Traced
algorithm_from_zig_string→ZigString::eql_comptime(pure byte compare, no JS entry) and confirmed the precedingget_zig_string(...)?propagates any exception, sohas_exception()is always false at both verify sites. - Traced
evp::lookup_ignore_case(ComptimeStringMap probe) andAlgorithm::md()(static BoringSSL singleton match) — neither can throw, so the PBKDF2 guard's else-branch is unreachable. - Checked the hoisted
sliceinPBKDF2::from_jsstays live for the error-message use and drops correctly onbreak 'brk; the removedJsErrorimport has no other use in the file.
Extended reasoning...
Overview
This PR removes three !global.has_exception() guards in src/runtime/crypto/PasswordObject.rs (js_password_object_verify, js_password_object_verify_sync) and src/runtime/crypto/PBKDF2.rs (PBKDF2::from_js). In each case the preceding fallible call (get_zig_string / to_slice) already propagates exceptions via ? on JsResult, and the code between that call and the guard is pure Rust (byte comparisons, a static string map lookup, a match returning BoringSSL const singletons) with no JS entry point. The else-branches (Err(JsError::Thrown) / Ok(JSValue::ZERO)) were therefore unreachable. The PBKDF2 change also hoists the slice binding one scope out so the error message reuses it instead of calling to_slice() a second time, and drops the now-unused JsError import.
Security risks
None. This is dead-code removal in argument-validation error paths. No new inputs are accepted, no validation is relaxed, and the crypto operations themselves (argon2/bcrypt/PBKDF2) are untouched.
Level of scrutiny
Low-to-moderate. The change is small (net −18 lines) and mechanical, but it touches crypto binding code, so I verified each unreachability claim against source: ZigString::eql_comptime at src/bun_core/string/mod.rs:1674 is a pure encoding-aware byte compare; evp::lookup_ignore_case at src/runtime/crypto/EVP.rs:160 is a ComptimeStringMap probe; Algorithm::md() at src/sha_hmac/sha.rs:248 is a match over static EVP_* getters. None of these can set a JS exception, so has_exception() is provably false after the preceding ?.
Other factors
Existing tests (password.test.ts "invalid algorithm throws", pbkdf2.test.ts "invalid inputs > digest") exercise the retained None/'invalid fallthroughs and pass per the PR description. The PR correctly notes a fail-before/pass-after test is structurally impossible for dead-branch removal, which REVIEW.md accepts. The slice lifetime hoist is sound — it lives to end of the 'brk block on both the success (break 'brk) and error (return Err) paths, and Rust's borrow checker enforces this. This aligns with REVIEW.md's "delete dead code in the same PR that makes it dead" and "simplest honest shape" guidance.
|
Closing. This is dead-branch removal with no observable behavior change: the The two related items from the same audit (the bcrypt |
What
Removes three
!global.has_exception()guards whose else-branches are provably unreachable:js_password_object_verify/js_password_object_verify_sync(PasswordObject.rs): the precedingarguments[2].get_zig_string(global_object)?propagates any exception via?, andalgorithm_from_zig_stringis a sequence ofZigString::eql_comptimebyte compares with no JS entry.has_exception()is always false at the guard, so theErr(JsError::Thrown)/Ok(JSValue::ZERO)fallthroughs are dead. TheJsErrorimport had no other use in the file.PBKDF2::from_js(PBKDF2.rs): the precedingarg4.to_slice(global_this)?propagates any exception via?;evp::lookup_ignore_caseis aComptimeStringMapprobe andAlgorithm::md()returns a static BoringSSL singleton pointer.has_exception()is always false andreturn Err(JsError::Thrown)is dead. The digestsliceis hoisted one scope out so the error message reuses it instead of callingto_slice()a second time.Why
These guards are leftovers from the pattern where a fallible helper returned an in-band sentinel (
None/.zero) with the exception left pending for the caller to check. In the Rust bindings,?onJsResultalready propagates the exception, so the explicithas_exception()check always observesfalseand the alternate branch is unreachable. Keeping them suggests a pending-exception state that cannot exist and invites readers to audit for one.Verification
No behavior change. Existing coverage exercises every touched path:
test/js/bun/util/password.test.ts"invalid algorithm throws" passes"scrpyt"to bothverifyandverifySync, hitting theNonearm ofalgorithm_from_zig_string.test/js/node/crypto/pbkdf2.test.ts"invalid inputs > digest" passes"md55", hitting the'invalidfallthrough and assertingInvalid digest: md55.Both suites pass on this branch (106 pass, 0 fail). Ran the password suite under
BUN_JSC_validateExceptionChecks=1as well.Because the removed branches are dead, there is no input that distinguishes before from after, so this change cannot produce a fail-before/pass-after test. Related items from the same audit (the redundant
& 0x3Fmask on bcrypt cost, and the explicitdrop()calls in asyncverify) are left out here since #33865 and #33702 already rewrite those lines as part of behavioral fixes.